[PM-33436] Refactor setup shell commands#7494
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7494 +/- ##
=======================================
Coverage 59.13% 59.13%
=======================================
Files 2077 2077
Lines 91848 91848
Branches 8175 8175
=======================================
+ Hits 54315 54316 +1
+ Misses 35601 35600 -1
Partials 1932 1932 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a cert with a non-utc `NotBefore` is not considered in range by xUnit's assert helper.
| var hundredYearsFromNow = DateTime.UtcNow.AddDays(36500); | ||
| var hundredYearsFromNow = DateTime.Now.AddDays(36500); | ||
|
|
||
| Assert.InRange(cert.NotAfter, hundredYearsFromNow.AddMinutes(-1), hundredYearsFromNow.AddMinutes(1)); |
There was a problem hiding this comment.
When using an Ubuntu VM, this assert would fire seemingly because the cert's NotAfter date would include the timezone I set for the VM. This test may have originally been run in a Docker container with the timezone left as UTC, so comparing with UtcNow wasn't a problem. Using Now instead should still work for the container test scenario.
[xUnit.net 00:00:02.56] Setup.Test.ProgramTests.Install_Works [FAIL]
Failed Setup.Test.ProgramTests.Install_Works [2 s]
Error Message:
Assert.InRange() Failure: Value not in range
Range: (2126-03-24T19:04:58.3800983Z - 2126-03-24T19:06:58.3800983Z)
Actual: 2126-03-24T14:05:56.0000000-05:00
Stack Trace:
at Setup.Test.ProgramTests.Install_Works() in /home/parallels/src/server/test/Setup.Test/ProgramTests.cs:line 77
--- End of stack trace from previous location ---
| public static string Exec(string cmd, string[] arguments = null, bool returnStdout = false, bool returnStderr = false) | ||
| { | ||
| var process = new Process | ||
| var startInfo = new ProcessStartInfo(cmd, arguments ?? []) |
There was a problem hiding this comment.
This change is the main reason for this PR, passing arguments as an ArgumentList instead of as a single string.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR refactors the Setup utility to invoke external processes via Code Review DetailsNo findings above the reporting threshold. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
djsmith85
left a comment
djsmith85
left a comment
There was a problem hiding this comment.
Changes are looking great.
Not sure why the linter is failing, but might run it locally and see if any changes come up. I assume it might be due to not using {} on if-statements.
Let me know once it's ready for re-review and I'll approve.
|
I was also not certain why it was failing. I merged |
2 participants
🎟️ Tracking
PM-33436
📔 Objective
Remove the Setup utility's use of
bash -c "<command>"orpwsh <command>, as well as reduce risk of command injection. This is done by using theProcessStartInfo.ArgumentListarray property instead of theArgumentsstring property.The majority of the PR is updating usages of
Helpers.Execand dealing withstdout/stderrdue to a few shell commands' input redirection having been removed.